Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix constructor spy in unit test with Sinon 4.1.3 #7433

Merged

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Dec 8, 2017

TL;DR;

When spying on a constructor using Sinon the objects created by the constructor are got using spy.getCall(XXX).returnValue.

All the long and boring details

As explained in the MDN Web Docs, new Foo(...) is executed in three steps:

  1. First a new object that inherits from Foo.prototype is created.
  2. Then the constructor function Foo is called with this bound to that new object.
  3. Finally the object returned by the constructor, or the object created in the first step if none is returned, becomes the result of the whole new expression.

When a constructor (or any other function) is spied using Sinon the original constructor is replaced by a proxy. When the proxy is called it will record the arguments it was called with, call the original constructor, and finally record the value returned or the exceptions thrown by the original constructor.

Before Sinon 4.1.3 when the proxy was called using the new operator the proxy called the original constructor directly, passing it the thisValue, which was the object created in step 1 above. If the original constructor function did not return an object (and note that this is the most common case, as the value returned by a constructor is usually undefined; as explained above new is the one that returns the created object when the constructor returns no value) then returnValue was set to thisValue; this is the value returned by the proxy to new, and thus is the value returned by the whole new expression as explained in the step 3. Therefore, before Sinon 4.1.3, spy.getCall(XXX).thisValue and spy.getCall(XXX).returnValue had (if the constructor returned no value, like in the failing test) the same value.

This has changed in Sinon 4.1.3, as now when the proxy is called using the new operator the proxy calls the original constructor using new too. When new is called inside the proxy the three steps outlined above happen again: a new object is created, the original constructor is called with this bound to that object, and the object is returned as the result of the whole new (bind.apply(this.func || func, [thisValue].concat(args)))() expression; as returnValue is an object it is not replaced by thisValue and thus returnValue is the value returned by the proxy to the new operator that called it.

Hey, wait a minute, when the original constructor is called inside the proxy it is bound to thisValue, so when new calls the original constructor in the step 2 it will use thisValue instead of the newly created object as this, right? Nope, as explained again in the MDN Web Docs when a function is bound this is ignored if the function is called by new.

Therefore, in Sinon 4.1.3, thisValue contains the object created by new proxy, but returnValue contains the real object created by new originalConstructor inside the proxy and also returned by the outermost new expression. That is, returnValue is the one you want to use.

Seriously, are you still reading this? Well, thanks :-) It is great that someone else besides me read it; it seems that I did not waste my time after all :-P

When a constructor is spied using Sinon it is wrapped by a proxy
function, which calls the original constructor when invoked. When "new
Foo()" is executed a "Foo" object is created, "Foo" is invoked with the
object as "this", and the object is returned as the result of the whole
"new" expression.

Before Sinon 4.1.3 the proxy called the original constructor directly
using the "thisValue" of the spied call; "thisValue" was the object
created by the "new" operator that called the proxy. The proxy assigned
"thisValue" to "returnValue", so it was also the value returned by the
proxy and, in turn, the value returned by the whole "new" expression.

Since Sinon 4.1.3 (see pull request 1626) the proxy calls the original
constructor using "new" instead of directly. The "thisValue" created by
the outermost "new" (the one that called the proxy) is no longer used by
the original constructor; the internal "new" creates a new object, which
is the one passed to the original constructor and returned by the
internal "new" expression. This object is also the value returned by the
proxy ("returnValue") and, in turn, the value returned by the whole
outermost "new" expression.

Thus, now "returnValue" should be used instead of "thisValue" to get the
object created by the spied constructor.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu added the 3. to review Waiting for reviews label Dec 8, 2017
@danxuliu danxuliu added this to the Nextcloud 13 milestone Dec 8, 2017
@danxuliu danxuliu requested a review from MorrisJobke December 8, 2017 20:45
@codecov
Copy link

codecov bot commented Dec 8, 2017

Codecov Report

Merging #7433 into master will increase coverage by 0.51%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7433      +/-   ##
============================================
+ Coverage     50.42%   50.94%   +0.51%     
  Complexity    24717    24717              
============================================
  Files          1522     1586      +64     
  Lines         86432    94163    +7731     
  Branches          0     1365    +1365     
============================================
+ Hits          43587    47971    +4384     
- Misses        42845    46192    +3347
Impacted Files Coverage Δ Complexity Δ
lib/private/Security/CertificateManager.php 91.08% <0%> (-1%) 39% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.28% <0%> (-0.25%) 136% <0%> (ø)
core/js/systemtags/systemtagsmappingcollection.js 68.42% <0%> (ø) 0% <0%> (?)
core/js/shareitemmodel.js 87.22% <0%> (ø) 0% <0%> (?)
core/js/sharedialoglinkshareview.js 59.21% <0%> (ø) 0% <0%> (?)
core/js/sharedialogshareelistview.js 44.91% <0%> (ø) 0% <0%> (?)
apps/comments/js/commentstabview.js 84.16% <0%> (ø) 0% <0%> (?)
settings/js/users/deleteHandler.js 96.05% <0%> (ø) 0% <0%> (?)
apps/files_versions/js/versionstabview.js 86.84% <0%> (ø) 0% <0%> (?)
core/js/eventsource.js 7.69% <0%> (ø) 0% <0%> (?)
... and 59 more

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seriously, are you still reading this? Well, thanks :-) It is great that someone else besides me read it; it seems that I did not waste my time after all :-P

😜

Anyways thanks a lot 👍

@MorrisJobke MorrisJobke merged commit 3f34861 into master Dec 8, 2017
@MorrisJobke MorrisJobke deleted the fix-constructor-spy-in-unit-test-with-sinon-4.1.3 branch December 8, 2017 22:49
@ProLoser
Copy link

ProLoser commented Dec 8, 2017

I'm confused. Is the new behavior better or worse?

@danxuliu
Copy link
Member Author

@ProLoser

I'm confused. Is the new behavior better or worse?

I would say just different. According to your pull request calling new on the spied constructor is needed to instantiate ES6 objects, but that prevents spying the this value used in the constructor (if the constructor returned a different value than this); whether that is better or worse depends on your needs ;-)

@MorrisJobke
Copy link
Member

stable12 backport: #7451

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants